Skip to content

docs: Update presto.default-namespace description #26625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

docs: Update presto.default-namespace description #26625

wants to merge 1 commit into from

Conversation

@PingLiuPing
Copy link
Contributor

@PingLiuPing PingLiuPing commented Nov 14, 2025

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
  • If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 14, 2025
@PingLiuPing PingLiuPing marked this pull request as ready for review November 14, 2025 14:28
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Nov 14, 2025 •
edited
Loading

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This PR refines the documentation for the ’presto.default-namespace’ property in the C++ connector’s Sphinx docs by clarifying its behavior and aligning its description style with other configuration entries.

File-Level Changes

Change Details Files
Refined presto.default-namespace property description
  • Clarified default namespace semantics and usage
  • Reworded text for improved readability
  • Aligned description format with other connector properties
 presto-docs/src/main/sphinx/presto_cpp/properties.rst
Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@PingLiuPing PingLiuPing requested review from a team, elharo and steveburnett as code owners November 14, 2025 14:28
@prestodb-ci prestodb-ci requested review from a team, ShahimSharafudeen and bibith4 and removed request for a team November 14, 2025 14:28
@github-actions github-actions bot added the docs label Nov 14, 2025
@github-project-automation github-project-automation bot moved this to 🆕 Unprioritized in Presto Documentation Nov 14, 2025
@PingLiuPing
Copy link
Contributor Author

PingLiuPing commented Nov 14, 2025

Screenshot 2025-11-14 at 13 12 47

sourcery-ai[bot]
sourcery-ai bot reviewed Nov 14, 2025
View reviewed changes
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@PingLiuPing
Copy link
Contributor Author

PingLiuPing commented Nov 14, 2025

I feel that this documentation belongs to Prestissimo as this is where the functions are registered, no?

I also feel that something is missing... The original issue was that a Connector was registering functions for its own use and somehow that caused failures in Prestissimo. What happened? Is there an implicit requirement from Prestissimo on Connector that's not yet documented?

@mbasmanova In Prestissimo, it retrieves all registered functions. And do some filter based on the catalog part of the function names and do further logic based on schema name. Here it takes the assumption that all function are registered with pattern catalog.schema. See code

presto/presto-native-execution/presto_cpp/main/functions/FunctionMetadata.cpp

Lines 256 to 321 in 9ddd6e7

json getFunctionsMetadata(const std::optional<std::string>& catalog) {
json j;
// Lambda to check if a function should be skipped based on catalog filter
auto skipCatalog = [&catalog](const std::string& functionCatalog) {
return catalog.has_value() && functionCatalog != catalog.value();
};
// Get metadata for all registered scalar functions in velox.
const auto signatures = getFunctionSignatures();
static const std::unordered_set<std::string> kBlockList = {
"row_constructor", "in", "is_null"};
// Exclude aggregate companion functions (extract aggregate companion
// functions are registered as vector functions).
const auto aggregateFunctions = exec::aggregateFunctions().copy();
for (const auto& entry : signatures) {
const auto name = entry.first;
// Skip internal functions. They don't have any prefix.
if (kBlockList.count(name) != 0 ||
name.find("$internal$") != std::string::npos ||
getScalarMetadata(name).companionFunction) {
continue;
}
const auto parts = util::getFunctionNameParts(name);
if (skipCatalog(parts[0])) {
continue;
}
const auto schema = parts[1];
const auto function = parts[2];
j[function] = buildScalarMetadata(name, schema, entry.second);
}
// Get metadata for all registered aggregate functions in velox.
for (const auto& entry : aggregateFunctions) {
if (!aggregateFunctions.at(entry.first).metadata.companionFunction) {
const auto name = entry.first;
const auto parts = util::getFunctionNameParts(name);
if (skipCatalog(parts[0])) {
continue;
}
const auto schema = parts[1];
const auto function = parts[2];
j[function] =
buildAggregateMetadata(name, schema, entry.second.signatures);
}
}
// Get metadata for all registered window functions in velox. Skip aggregates
// as they have been processed.
const auto& functions = exec::windowFunctions();
for (const auto& entry : functions) {
if (aggregateFunctions.count(entry.first) == 0) {
const auto name = entry.first;
const auto parts = util::getFunctionNameParts(entry.first);
if (skipCatalog(parts[0])) {
continue;
}
const auto schema = parts[1];
const auto function = parts[2];
j[function] = buildWindowMetadata(name, schema, entry.second.signatures);
}
}
return j;
}
.

During parse the function name to get the catalog and schema name. It uses

VELOX_USER_CHECK(
      parts.size() == 3,
      fmt::format("Prefix missing for function {}", registeredFunction));

to check the function has 3 part names.

@czentgr @amitkdutta Could you help to add more info how above assertion fail can lead to worker node crash? I don't see callstack in #26584. Thanks.
Can we skip those functions that do not have such prefix pattern instead of fail immediately?

steveburnett
steveburnett requested changes Nov 14, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation!

@github-project-automation github-project-automation bot moved this from 🆕 Unprioritized to 🏗 In progress in Presto Documentation Nov 14, 2025
@PingLiuPing PingLiuPing mentioned this pull request Nov 14, 2025
Open
aditi-pandit
aditi-pandit previously approved these changes Nov 14, 2025
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed for content. Sounds good.

@PingLiuPing
Copy link
Contributor Author

PingLiuPing commented Nov 21, 2025

@steveburnet Updated the code, would you have another look, thank you.

steveburnett
steveburnett requested changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision! Just one nit of phrasing.

presto-docs/src/main/sphinx/presto_cpp/properties.rst

The configured value (for example, ``presto.default``) is automatically appended with a
trailing dot (``.``) to form the complete prefix (``presto.default.``). This results
in fully qualified function names like ``presto.default.substr`` or ``presto.default.sum``. Note, internal functions (prefixed with ``$internal$``) do not follow this pattern and are exempt from the three-part naming requirement.
Copy link
Contributor

@steveburnett steveburnett Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in fully qualified function names like ``presto.default.substr`` or ``presto.default.sum``. Note, internal functions (prefixed with ``$internal$``) do not follow this pattern and are exempt from the three-part naming requirement.
in fully qualified function names like ``presto.default.substr`` or ``presto.default.sum``. Internal functions (prefixed with ``$internal$``) do not follow this pattern and are exempt from the three-part naming requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@steveburnett steveburnett steveburnett requested changes

@aditi-pandit aditi-pandit aditi-pandit left review comments

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@bibith4 bibith4 Awaiting requested review from bibith4 bibith4 was automatically assigned from prestodb/ibm-reviewers

@ShahimSharafudeen ShahimSharafudeen Awaiting requested review from ShahimSharafudeen ShahimSharafudeen was automatically assigned from prestodb/ibm-reviewers

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

docs from:IBM PR from IBM

Projects

Presto Documentation
Status: 🏗 In progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PingLiuPing @steveburnett @aditi-pandit @prestodb-ci